Open
Conversation
kris-gaudel
reviewed
Sep 23, 2025
| attr_reader :name, :pid_controller, :ping_thread | ||
|
|
||
| def initialize(name:, kp: 1.0, ki: 0.1, kd: 0.0, | ||
| window_size: 10, history_duration: 3600, |
Contributor
There was a problem hiding this comment.
Regarding the calculation of online mean, Welford's algorithm was discussed as a solution to use constant space
Contributor
There was a problem hiding this comment.
Also, do we want to allow developers to configure the interval of integration? (history_duration)?
4c9eb68 to
550a46f
Compare
69ab5b7 to
550a46f
Compare
Update variable names Fill sliding window with 1 hr worth data Add comment Update experiemnt resource to be deterministic Change deterministic default value to false Cleanup Remove unused variable Make initial seed error rate more customizable Add seed_error_rate as a property
* Prefilling added * Change initial duration to 900 s
* testing different circuit breaking scenarios * adding concurrency * adds more puts to get further information during phases * Fixing concurrency, unprotected ping, extras * update classic sustained test * cleaning up outdated tests, and testing without ping rate * modify ki instead of dividing by window size --------- Co-authored-by: Abdulrahman Alhamali <abdulrahman.alhamali@shopify.com>
d52fa4d to
56ed006
Compare
* allow using the fiber scheduler if present * run experiments
…oved calculations
…rror-rate-calculation Fixed redundant ideal error rate calculation. Calculated once and re-used appropriately.
* Added param to only use 'current_window_requests' when needed. Updated 'start_pid_controller_update_thread' to only pass in needed values to 'notify_metrics_update'. * Removed notifying logic for state transitions. * Refactor adaptive circuit breaker tests by removing the state transition notification test. Updated metrics expectations to include previous p_value and adjusted test logic for clarity. * Added updated experiments
Contributor
|
Hey guys, hope you had a good start to the new year! Are there any issue I could work on in an open-source capacity on the ACB? |
| end | ||
|
|
||
| def wait_for_window | ||
| Kernel.sleep(@sliding_interval) |
There was a problem hiding this comment.
Suggested change
| Kernel.sleep(@sliding_interval) | |
| Kernel.sleep(@sliding_interval * rand(0.9...1.1) |
|
Here are the notes from our tuple: |
* add jitter to sleep * fix tests and re-run experiments --------- Co-authored-by: Adrian Gudas <adrian.gudas@shopify.com>
stop thread if the last remaining circuit breaker is destroyed use a sliding interval of 1 (run through the loop every second)
…hread using a single thread for all PID controller statuses
* Add dead zone ratio to PID controller for noise suppression - Introduced `dead_zone_ratio` parameter in the PID controller to suppress noise from small deviations in error rates. - Updated the `calculate_p_value` method to implement dead zone logic, allowing for more stable control responses. - Enhanced tests to validate the behavior of the dead zone in various scenarios, ensuring it does not impede recovery while effectively filtering noise. * Added experiment tests * Refine dead zone logic in PID controller - Updated the handling of the dead zone in the PID controller to allow full signal response above the dead zone, improving control accuracy. - Adjusted comments for clarity regarding the purpose of the dead zone in noise suppression. * Added experiment results
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add a new adaptive circuit breaker that:
The circuit breaker has two main components:
Ideal Error Rate Estimator
This estimator tries to find out the expected error rate from a healthy dependency. It does that through a method of simple exponential smoothing, which basically relies on calculating a weighted average, with a few extra domain-specific hints:
PID Controller (Process, Integral, Derivative) Controller
See wikipedia link
This controller increases/decreases the the rejection rate of the circuit breaker based on the value of:
Where P is:
The intuition of the value of P:
Note: The derivate component is currently set to 0. The Integral component mostly contributes as a history to prevent the circuit breaker from fluctuating too much.